Skip to content

Comments

Add blake3 NEON instructions on linux arm64#19384

Closed
mauriciogg wants to merge 4 commits intobazelbuild:masterfrom
mauriciogg:patch-5
Closed

Add blake3 NEON instructions on linux arm64#19384
mauriciogg wants to merge 4 commits intobazelbuild:masterfrom
mauriciogg:patch-5

Conversation

@mauriciogg
Copy link
Contributor

@mauriciogg mauriciogg commented Aug 31, 2023

otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see d0de5e0

@mauriciogg
Disable using NEON instructions on linux arm64
36ad037 
otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see bazelbuild@d0de5e0
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 31, 2023
brentleyjones
brentleyjones reviewed Aug 31, 2023
View reviewed changes
third_party/blake3/blake3.BUILD Outdated
Comment on lines 60 to 62
"@bazel_tools//src/conditions:linux_aarch64": [
"-DBLAKE3_USE_NEON=0",
],
Copy link
Contributor

@brentleyjones brentleyjones Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we instead include c/blake3_neon.c like we do in the @bazel_tools//src/conditions:darwin_arm64 condition above, and set -DBLAKE3_USE_NEON=1? Same for windows arm64 @meteorcloudy.

Copy link
Contributor Author

@mauriciogg mauriciogg Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that works, I can make the change for windows here as well, but I'll wait for @meteorcloudy comments.

Copy link
Member

@meteorcloudy meteorcloudy Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, /cc @tylerwilliams who made the blake3 contribution in #18682

Copy link
Contributor

@brentleyjones brentleyjones Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tyler gave me the tip. I think we should do the Windows path as well.

Copy link
Contributor

@tylerwilliams tylerwilliams Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this change looks good to me too

@mauriciogg mauriciogg changed the title Disable using NEON instructions on linux arm64 Add blake3 NEON instructions on linux arm64 Aug 31, 2023
@iancha1992 iancha1992 added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Sep 1, 2023
@meteorcloudy meteorcloudy requested a review from coeuvre September 1, 2023 09:25
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 1, 2023
@copybara-service copybara-service bot closed this in 31812fe Sep 4, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 4, 2023
@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 4, 2023

Can you please also sync the blake3 BUILD file change to https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/blake3 ?

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 4, 2023 •
edited
Loading

You can have a new version named 1.3.3.bcr.1 as a patched version. See https://github.com/bazelbuild/bazel-central-registry/blob/main/docs/README.md#versions-format

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 4, 2023

Context: I'm working on #19087, so in future, blake3 will come from the BCR.

mauriciogg added a commit to mauriciogg/bazel-central-registry that referenced this pull request Sep 5, 2023
@mauriciogg
Add neon files to arm architectures
e47cb66 
see bazelbuild/bazel#19384
@mauriciogg mauriciogg mentioned this pull request Sep 5, 2023
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Oct 12, 2023
@mauriciogg @bazel-io
Add blake3 NEON instructions on linux arm64
f047c96 
otherwise getting libunix_jni.so: undefined symbol: blake3_hash_many_neon

see bazelbuild@d0de5e0

Closes bazelbuild#19384.

PiperOrigin-RevId: 562479599
Change-Id: I8f0ca6b68486f5ea8db698f3f99526eb1c0de8a8
@bazel-io bazel-io mentioned this pull request Oct 12, 2023
Merged
keertk pushed a commit that referenced this pull request Oct 12, 2023
@bazel-io @mauriciogg
[6.4.0] Add blake3 NEON instructions on linux arm64 (#19804)
ceb4f2d 
otherwise getting libunix_jni.so: undefined symbol:
blake3_hash_many_neon

see
d0de5e0

Closes #19384.

Commit
31812fe

PiperOrigin-RevId: 562479599
Change-Id: I8f0ca6b68486f5ea8db698f3f99526eb1c0de8a8

Co-authored-by: Mauricio G <up.mauricio.g@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@coeuvre coeuvre coeuvre approved these changes

+2 more reviewers

@tylerwilliams tylerwilliams tylerwilliams left review comments

@brentleyjones brentleyjones brentleyjones left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

team-Remote-Exec Issues and PRs for the Execution (Remote) team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mauriciogg @meteorcloudy @tylerwilliams @brentleyjones @coeuvre @iancha1992